Hi

I really need this feature. could you please take a look?

@olaservo @sebastien-rosset @Finndersen @domdomegg

I just resolved the merge conflicts in my previously submitted PR. Could you please review it to help prevent future conflicts?

@claude review

Claude finished @cliffhall's task in 1m 43s —— View job

PR Review: feat(filesystem): add append_file and write_or_update_file tools

• Gather context and understand request
• Read changed files
• Review lib.ts changes
• Review index.ts changes
• Review test changes
• Review README changes
• Post comprehensive feedback

Thanks for this PR, @agn-7. The feature direction is sound — there's a real gap between "always overwrite" and "no append option." A few issues need addressing before merge.

Critical Issues

1. Memory-inefficient read-modify-write in appendFileContent (lib.ts:187-215)

Both new functions read the entire file into memory, concatenate, then write atomically. For their stated use case (log files, incrementally-built documents), this is O(n) memory in file size — exactly where you'd expect large files. Node's fs.appendFile with the 'a' flag is designed for this and would be far simpler:

export async function appendFileContent(filePath: string, content: string): Promise<void> {
  try {
    await fs.access(filePath);
  } catch (error) {
    if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
      throw new Error(`File does not exist: ${filePath}`);
    }
    throw error;
  }
  await fs.appendFile(filePath, content, 'utf-8');
}

The existing atomic-rename pattern in writeFileContent makes sense because it prevents symlink-swap attacks on a full overwrite. For appending, fs.appendFile opens with O_WRONLY|O_CREAT|O_APPEND which is atomic at the OS level for the append operation itself. The upstream validatePath has already resolved symlinks, so the extra complexity isn't justified here. Fix this →

2. TOCTOU race between fs.access and fs.readFile (lib.ts:188-199)

fs.access(filePath)   ← file exists here
                      ← file deleted by another process
fs.readFile(filePath) ← throws ENOENT unexpectedly

The check-then-act pattern is inherently racy. Cleaner approach — just attempt the read directly:

let existingContent: string;
try {
  existingContent = await readFileContent(filePath);
} catch (error) {
  if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
    throw new Error(`File does not exist: ${filePath}`);
  }
  throw error;
}

This eliminates the race and removes a redundant syscall.

3. Unreliable birthtime === mtime heuristic (index.ts:416-419)

const message = stats.birthtime.getTime() === stats.mtime.getTime()
  ? `Successfully created ${args.path} with content`
  : `Successfully appended content to ${args.path}`;

After an atomic rename(), the target file's metadata reflects the temp file that replaced it — birthtime and mtime will almost never be equal on Linux (where birthtime support varies by filesystem). This heuristic will give wrong feedback in nearly all cases. Track file existence before the operation instead:

const existed = await fs.access(validPath).then(() => true).catch(() => false);
await writeOrUpdateFileContent(validPath, args.content);
const message = existed
  ? `Successfully appended content to ${args.path}`
  : `Successfully created ${args.path} with content`;

Fix this →

Design Issues

4. Code duplication (lib.ts:229-247)

The if (fileExists) branch in writeOrUpdateFileContent is a verbatim copy of the core logic in appendFileContent. If the atomic-write block changes (e.g., a bug fix), it needs updating in two places. The fix branch should just call appendFileContent:

if (fileExists) {
  await appendFileContent(filePath, content);
} else {
  await writeFileContent(filePath, content);
}

5. Semantics of write_or_update_file name (index.ts:394)

"Update" typically implies overwriting or modifying existing content. The actual behavior here is "create or append," which may surprise users who interpret "update" as replacing content. Consider create_or_append_file for clarity, though this is a judgment call and any name change should be weighed against discoverability. At minimum, the description is clear, so this is low priority.

Minor Issues

6. Three identical Zod schemas (index.ts:105-118)

WriteFileArgsSchema, AppendFileArgsSchema, and WriteOrUpdateFileArgsSchema are identical { path, content } shapes. These can share a single schema definition to reduce repetition.

7. Tests verify mock calls, not behavior

The new tests (e.g., 'appends content to existing file') assert that the right mock functions were called with the right arguments but don't test that the resulting file content is correct end-to-end. This is consistent with the existing test style in this file, so it's not a blocker, but the tests would catch more bugs if at least a couple used real temp files (similar to how the Python servers test).

What Looks Good

• The atomic rename pattern carried over from writeFileContent is the right idea
• Tool annotations (readOnlyHint, destructiveHint, idempotentHint) are set correctly
• README is updated with the new tools and the annotations table
• Error handling for non-ENOENT errors propagates correctly
• validatePath is called before these functions (ensuring security checks precede file operations)

Pushed 54ed837 addressing the review. Per-item response:

1. Memory-inefficient read-modify-write — fixed. appendFileContent now calls fs.appendFile(path, content, { encoding: 'utf-8', flag: 'a' }) directly. validatePath already resolves symlinks before this point, so the OS-level O_APPEND is sufficient and we drop the temp-file dance.

2. TOCTOU race — fixed. Removed the fs.access precheck. We attempt the append directly and translate ENOENT into the existing "File does not exist" error. One syscall, no race window.

3. Unreliable birthtime === mtime heuristic — fixed. The write_or_update_file handler now records existed via fs.access(...).then(()=>true).catch(()=>false) before the write and uses that boolean to pick the response message.

4. Code duplication — fixed. writeOrUpdateFileContent now calls appendFileContent first and falls back to writeFileContent only on the File does not exist error. Single source of truth for the append path.

6. Three identical Zod schemas — consolidated into PathContentArgsSchema, with WriteFileArgsSchema / AppendFileArgsSchema / WriteOrUpdateFileArgsSchema aliasing it for readability at the call sites.

5. write_or_update_file naming — leaving as-is for now. Renaming would be a breaking change for users who may already have wired this tool name from the existing PR; happy to rename to create_or_append_file if maintainers prefer.

7. Mock-based tests — left as-is, per the reviewer's note that this matches the existing style. Tests were updated to reflect the new direct-fs.appendFile implementation; all 153 filesystem tests pass.

cc @cliffhall @olaservo

Follow-up on item 5: rename applied in ebc44b2.

write_or_update_file → create_or_append_file (and the corresponding WriteOrUpdateFileArgsSchema / writeOrUpdateFileContent symbols renamed to match). Reasoning: since the PR has not merged yet, there are no real users wired to the old name, so the discoverability concern outweighs backward-compatibility. The new name now reflects the actual semantics (create-or-append, not update/overwrite). README and append_file description updated; all 153 tests still pass.